Governance: Votes #552
Governance: Votes #552
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughIntroduces a comprehensive voting system for the Soroban ecosystem with delegation, checkpointing, and voting unit tracking. Adds governance votes module with historical vote queries, fungible and non-fungible token voting extensions integrating with voting infrastructure, and an example contract demonstrating usage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Account as Account Storage
participant Votes as Votes Module
participant Delegate as Delegate Tracking
participant Checkpoints as Checkpoint Storage
participant History as Historical Queries
User->>Votes: delegate(account, delegatee)
Votes->>Account: verify authorization
Votes->>Delegate: update delegatee reference
Votes->>Checkpoints: push checkpoint for old delegate
Votes->>Checkpoints: push checkpoint for new delegate
Votes->>User: emit DelegateChanged
User->>Votes: transfer_voting_units(from, to, amount)
Votes->>Delegate: get old_delegate (from)
Votes->>Delegate: get new_delegate (to)
Votes->>Checkpoints: update old_delegate checkpoint
Votes->>Checkpoints: update new_delegate checkpoint
Votes->>User: emit DelegateVotesChanged
User->>History: get_past_votes(account, timepoint)
History->>Checkpoints: binary search checkpoint at timepoint
History->>User: return historical voting power
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
- Coverage 96.08% 96.00% -0.09%
==========================================
Files 54 57 +3
Lines 5215 5500 +285
==========================================
+ Hits 5011 5280 +269
- Misses 204 220 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/governance/src/votes/storage.rs`:
- Around line 258-312: The transfer_voting_units function currently calls
move_delegate_votes twice (once for sender and once for receiver), which emits
misleading zero-net events when both accounts delegate to the same address;
change the flow to first compute from_delegate = get_delegate(e, from_addr) and
to_delegate = get_delegate(e, to_addr) (for Option cases) and then invoke
move_delegate_votes once with those delegate Option<&Address> values so its
internal from==to short-circuit prevents redundant events; ensure you still
perform voting unit arithmetic (get_voting_units, checked_sub/checked_add,
set_voting_units) and still call push_total_supply_checkpoint for mint/burn
paths, but move the delegate lookup so a single call to move_delegate_votes
replaces the two separate calls.
🧹 Nitpick comments (3)
packages/governance/README.md (1)
118-121: Consider adding the fungible-votes example.The Examples section only references the timelock-controller example. Consider adding a reference to the new
examples/fungible-votes/example to showcase the Votes module usage.📝 Suggested addition
## Examples See the following examples in the repository: +- [`examples/fungible-votes/`](https://github.com/OpenZeppelin/stellar-contracts/tree/main/examples/fungible-votes) - Fungible token with voting power delegation - [`examples/timelock-controller/`](https://github.com/OpenZeppelin/stellar-contracts/tree/main/examples/timelock-controller) - Timelock controller with role-based access controlexamples/fungible-votes/src/contract.rs (1)
1-8: Unused import:MuxedAddress.The
MuxedAddresstype is imported but not used in this file.♻️ Suggested fix
-use soroban_sdk::{contract, contractimpl, Address, Env, MuxedAddress, String}; +use soroban_sdk::{contract, contractimpl, Address, Env, String};packages/governance/src/votes/storage.rs (1)
316-324: Addextend_ttlafter persistent storage writes to prevent premature expiration of voting data.The codebase consistently calls
extend_ttlafter.get()operations (visible inget_delegate,get_voting_units,get_checkpoint,get_total_supply_checkpoint), but does not call it after.set()operations inset_voting_units,delegate, or checkpoint creation/updates. Since.set()initializes entries with the network minimum TTL, there's a window where newly written data could expire before being read and extended. For critical historical voting data, extend TTL immediately after writes to ensure consistent persistence.♻️ Example tweak for this helper
fn set_voting_units(e: &Env, account: &Address, units: u128) { let key = VotesStorageKey::VotingUnits(account.clone()); if units == 0 { e.storage().persistent().remove(&key); } else { e.storage().persistent().set(&key, &units); + e.storage().persistent().extend_ttl(&key, VOTES_TTL_THRESHOLD, VOTES_EXTEND_AMOUNT); } }Apply the same pattern to
delegate()and checkpoint creation inpush_checkpointandpush_total_supply_checkpoint.
| //! This module provides storage functions that can be integrated into a token | ||
| //! contract. The contract is responsible for: |
There was a problem hiding this comment.
I would rephrase here. This module is not only consisting of storage.rs. It also provides the trait, etc.
| //! delegate, get_votes, get_past_votes, transfer_voting_units, | ||
| //! }; | ||
| //! | ||
| //! // In your token contract transfer: |
There was a problem hiding this comment.
we should make here more clear. For example, override the transfer method as shown in below. From this paragraph alone, developer may not understand what to do
| /// | ||
| /// * `e` - Access to the Soroban environment. | ||
| /// * `account` - The address to query voting power for. | ||
| fn get_votes(e: &Env, account: Address) -> u128 { |
There was a problem hiding this comment.
this is great! I wanted to get rid of get_votes in the actual governor trait interface. I think it belongs here.
If this wasn't accidental, and if you are ok with removing get_votes from governor interface to make it slimmer, let's keep it like this.
Open for discussions, because I've made a lot of changes to the governor interface to make it more compact
| /// | ||
| /// * `e` - Access to the Soroban environment. | ||
| /// * `account` - The address to query voting power for. | ||
| fn get_votes(e: &Env, account: Address) -> u128 { |
There was a problem hiding this comment.
should we mention this will return 0 for addresses that do not exist (not a participant of this contract)? Because I don't see any error related to that. The same for other similar functions in the below
| //! | ||
| //! This module follows the design of OpenZeppelin's Solidity `Votes.sol`: | ||
| //! - Voting units must be explicitly delegated to count as votes | ||
| //! - Self-delegation is required for an account to use its own voting power |
There was a problem hiding this comment.
I think, highlighting only delegatees can vote here would be good, and also repeating it in storage.rs. I'll mention it in a comment there
| pub votes: u128, | ||
| } | ||
|
|
||
| /// Storage keys for the votes module. |
There was a problem hiding this comment.
"only delegatees can vote, so this storage key is designed around that principle" kind of reminder would be helpful. I got confused whilst reading this storage key, and then I remembered I read something similar in the mod.rs.
| /// | ||
| /// This is the total voting power delegated to this account by others | ||
| /// (and itself if self-delegated). | ||
| /// |
There was a problem hiding this comment.
Again, I think it is important here (and maybe in the corresponding trait method as well), this will return 0, if the voting power has not delegated to anyone (including self)
| /// # Errors | ||
| /// | ||
| /// * [`VotesError::FutureLookup`] - If `timepoint` >= current timestamp. | ||
| fn get_past_votes(e: &Env, account: Address, timepoint: u64) -> u128 { |
There was a problem hiding this comment.
I suggest renaming this to get_votes_at_checkpoint. Will be much more clear. The same for the storage function
| get_checkpoint(e, account, num - 1).votes | ||
| } | ||
|
|
||
| /// Returns the voting power of an account at a specific past timestamp. |
There was a problem hiding this comment.
timestamp or timepoint, should be consistent
| // Check if timepoint is after the latest checkpoint | ||
| let latest = get_total_supply_checkpoint(e, num - 1); | ||
| if latest.timestamp <= timepoint { | ||
| return latest.votes; | ||
| } | ||
|
|
||
| // Check if timepoint is before the first checkpoint | ||
| let first = get_total_supply_checkpoint(e, 0); | ||
| if first.timestamp > timepoint { | ||
| return 0; | ||
| } | ||
|
|
||
| // Binary search | ||
| let mut low: u32 = 0; | ||
| let mut high: u32 = num - 1; | ||
|
|
||
| while low < high { | ||
| let mid = (low + high).div_ceil(2); | ||
| let checkpoint = get_total_supply_checkpoint(e, mid); | ||
| if checkpoint.timestamp <= timepoint { | ||
| low = mid; | ||
| } else { | ||
| high = mid - 1; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
this is duplicated also in get_past_votes, maybe we can re-use this code piece in an helper function
| get_past_votes(e, &account, timepoint) | ||
| } | ||
|
|
||
| /// Returns the total supply of voting units at a specific past timestamp. |
There was a problem hiding this comment.
I'm confused about this one. Is this also only accounting for the delegated votes? Should be... But wanted to double check
| /// # Notes | ||
| /// | ||
| /// Authorization for `account` is required. | ||
| pub fn delegate(e: &Env, account: &Address, delegatee: &Address) { |
There was a problem hiding this comment.
this currently allows for specifying the same delegate, and overriding it with itself.
I don't think it is a security issue, but better to give an error to eliminate possible wrong transactions
| set_voting_units(e, from_addr, new_from_units); | ||
| } else { | ||
| // Minting: increase total supply | ||
| push_total_supply_checkpoint(e, true, amount); |
There was a problem hiding this comment.
I'd advice against having a bool that represents add or subtract.
May sound like an overkill, but it's not, let's create an enum for that and pass that concrete type here. Would be more readable and prevent possible bugs in the future
There was a problem hiding this comment.
The variants could be burn and mint
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
Liked the overall structure. Few changes are requested. All of them are fairly minimal.
Only reviewed the half of it. Will continue to review tomorrow. Reminder to myself: push_checkpoint
Fixes #437
Fixes #439
PR Checklist
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.